-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(langchain): use callbacks #1170
Conversation
162bca3
to
3638ab6
Compare
There is quite some duplication, maybe there is a better way to do this? |
@tibor-reiss I think a better way will actually be something like #902. I think the contributor stopped working on it there so potentially we can pick this up (I was planning to do so sometime next week) |
660f063
to
d732242
Compare
Hi @nirga, I have checked out the linked PR, and based on that (+research into callbacks) I managed to make it (in sync case) work for However, Note: for now, below seems to work, but it's explicit: I don't know how fast the API changes on langchain side, so not sure about the whole migration to callbacks via a decorator. Since there is the simple way of adding callbacks to e.g. Let me know your thoughts, please! Ps.: we can open a new PR for the callback part first, and then rework this later... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tibor-reiss this looks like a great step in the right direction! We should do something similar to our llamaindex instrumentation as well (we have an issue for that). I think this is still more robust than the way we do it today. I've been in touch with the folks from Langchain and hopefully we'll found a way for the new Runnable syntax.
I'll dig into their codebase as well to try to come up with a solution but I think your proposal here works well! (at least for now).
Mind fixing the lint / tests?
@@ -39,12 +46,8 @@ def test_sequential_chain(exporter): | |||
) | |||
|
|||
spans = exporter.get_finished_spans() | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing you want to keep the assertions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, ofc, cleaned up the PR now.
Potentially what we can do (maybe outside the scope for this PR) - wrap a call to |
660f063
to
6f6e2ee
Compare
Sounds like a good idea. I agree that smaller PRs seems advantageous here because of how fast langchain changes, and because the way forward is not so clear. I have now edited the scope/description of the PR. As you said, it's probably better to first implement callbacks as far as possible, and do later RunnableSequence. I'll try to add the async callback asap - but feel free to add it yourself :) For this PR, what might still make sense is to add BaseChatModel. It's not trivial due to 2 reasons:
|
@tibor-reiss so let's do it gradually. Begin with this PR, and think about I see you marked this as WIP, let me know when this is ready and I'll do a full review :) |
6f6e2ee
to
c923d35
Compare
@nirga please check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall @tibor-reiss! Great work :)
(I'm guessing we'll need to do it for other types of chains as well, right?)
|
||
spans = exporter.get_finished_spans() | ||
|
||
assert [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also test other parts of the callback instrumentation? For example, see the inputs and outputs are set on the spans, or give names to the LLMChain
s and see that the span name is set accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added more checks.
Re name: the span name is set in the callback handler, so it can't be changed (there might be occasions where it can be, e.g. with_config)
Hi @nirga , I was away due to some personal reasons. I have some changes parked in my local and wanted to review them, but it looks like there is some progress in this PR and I'm not sure if I should continue with my PR 😞 . Let me know if you want me to close that one. |
Maybe we can combine your work here as well @midhun1998 ? |
Works for me. I can add in my changes here or get my PR finished up quickly. |
3d7b763
to
c3816a2
Compare
Yes, chains can be added. I added now StuffDocumentsChain (with a test). Maybe there is a better way to add all children of the base class since Chain is already added and here is the callbacks attribute defined... @midhun1998 feel free to edit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tibor-reiss @midhun1998 I'm going to merge this so we can continue this work against main
branch - I think it will be easier. Great work @tibor-reiss :) Thanks so much!
Some work on #541 (which will then allow to fix #1101 in a more maintainable way)
feat(instrumentation): ...
orfix(instrumentation): ...
.ToDo: